Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe CLI now uses platform-native launchers to execute ChangesDirect Bun launcher migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
LLxprt PR Review – PR #2610Issue AlignmentResolves #2603 by removing the Node relay from the installed command path. POSIX installs now use a checked-in Side Effects
Code QualityThe implementation is thorough and defensive:
Tests and CoverageCoverage impact: increase New behavioral tests cover:
Tests exercise real installs, actual npm pack artifacts, and platform-specific behaviors rather than mocking implementation details. VerdictReady The PR directly implements #2603 with a well-scoped, defensive implementation. The removal of the Node relay is a meaningful architectural change that reduces startup overhead and Windows process-chain fragility. Test coverage is comprehensive and behavioral. The main residual risk is the complexity of the POSIX shell launcher, which is mitigated by shellcheck validation and extensive runtime tests. No blocking issues found. |
OpenCodeReview — PR #2610
Findings without a resolvable position
|
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/test-utils/src/cli-args.test.ts (1)
90-99: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd coverage for the non-installed Bun branch.
This test forces the installed
llxprtpath, so it never verifies the changedcommand: 'bun'and[entryPath, ...args]behavior. Add a case with the environment unset or false that asserts Bun and the TypeScript entry path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/test-utils/src/cli-args.test.ts` around lines 90 - 99, Add a separate test in the CLI argument tests that leaves INTEGRATION_TEST_USE_INSTALLED_LLXPRT unset or false, then call getCommandAndArgs with the TypeScript entry path and arguments and assert command is bun with initialArgs containing the entry path followed by those arguments. Keep the existing installed-binary/profile test unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/deployment.md`:
- Line 128: Update the release-process documentation to scope the no-`dist/`
statement specifically to the CLI package, while explicitly preserving that
internal workspace dependency packages still require compilation via `npm run
build:packages` and their `dist/` artifacts.
In `@docs/troubleshooting.md`:
- Line 280: Add an exit-code 43 row to the troubleshooting document’s Exit Codes
table, identifying it as a launcher-runtime failure caused by a missing or
unusable bundled Bun and directing users to the relevant launcher diagnostic.
In `@packages/cli/bin/llxprt`:
- Around line 16-35: The bun discovery loop in the llxprt launcher must not
accept an arbitrary ancestor executable. Update the candidate validation around
_llxprt_bun so each found bun resolves to this package’s bundled pinned
dependency, supporting both hoisted and pnpm node_modules layouts, and continue
searching or fail when it does not.
- Around line 45-64: Extend the runtime validation surrounding _llxprt_magic to
verify that the bundled binary is executable on the current POSIX host, not
merely that its header is recognized. Reject incompatible architectures and PE
binaries unsupported by the host or emulation environment with the existing
actionable exit-43 diagnostics, while retaining the final exec path so signals
and exit statuses propagate unchanged.
In `@README.md`:
- Around line 109-113: Update the “Bun resolution (production launcher)” list in
README.md to include node_modules/.bin/bun.exe as the third candidate,
preserving the documented order and ancestor-directory search behavior.
In `@scripts/postinstall.cjs`:
- Around line 277-281: Wrap the cliInstaller require and installNativeLaunchers
invocation in a try/catch so unexpected launcher-generation or module-loading
errors are logged via the existing console logger without aborting postinstall.
Preserve the current packageRoot and log options, and continue allowing
installNativeLaunchers’ returned error codes to be surfaced as already handled.
In `@scripts/tests/issue-2603-release-install.test.ts`:
- Line 187: Relax the stderr assertion in the release-install smoke test after
the existing status and success-marker checks: verify captured stderr does not
contain any “FAIL:” lines instead of requiring it to be empty, allowing benign
safeCleanup warnings while still detecting smoke-test failures.
In `@scripts/windows-installed-command-smoke/checks.cjs`:
- Around line 226-328: Extend the launcher checks so PowerShell is covered
alongside cmd for injection safety, bundled Bun exec-path validation, and
process-tree behavior. Update checkInjectionGuard and checkExecPathIsBundledBun
to invoke the existing invokePwsh helper with ps1Path, and mirror the relevant
process-tree check using its existing PowerShell path. Also ensure
checkPwshArgFidelity validates bunVersion like checkCmdArgFidelity.
- Around line 330-382: Update checkProcessTreeNoNode so readiness polling yields
to the event loop instead of using blocking sleepMs(100), allowing stdout and
exit handlers to update state during the timeout. Ensure cleanup terminates the
entire spawned process tree, including bun.exe, rather than only calling
child.kill() on the direct cmd.exe process.
---
Outside diff comments:
In `@packages/test-utils/src/cli-args.test.ts`:
- Around line 90-99: Add a separate test in the CLI argument tests that leaves
INTEGRATION_TEST_USE_INSTALLED_LLXPRT unset or false, then call
getCommandAndArgs with the TypeScript entry path and arguments and assert
command is bun with initialArgs containing the entry path followed by those
arguments. Keep the existing installed-binary/profile test unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b4b57460-edfa-4fab-af50-a4dd581f3d48
⛔ Files ignored due to path filters (8)
bun.lockis excluded by!**/*.lock,!**/*.lockdev-docs/npm.mdis excluded by!dev-docs/**package-lock.jsonis excluded by!**/package-lock.json,!package-lock.jsonscripts/tmux-script.approval-ui.jsonis excluded by!scripts/tmux-script.*.jsonscripts/tmux-script.issue2208-newlines.fake.jsonis excluded by!scripts/tmux-script.*.jsonscripts/tmux-script.lsp-diag-smoke.jsonis excluded by!scripts/tmux-script.*.jsonscripts/tmux-script.lsp-smoke.jsonis excluded by!scripts/tmux-script.*.jsonscripts/tmux-script.slash-autocomplete.jsonis excluded by!scripts/tmux-script.*.json
📒 Files selected for processing (48)
.github/workflows/ci.yml.github/workflows/interactive-ui.yml.github/workflows/nightly.yml.github/workflows/smoke-test.yml.github/workflows/windows-installed-command.yml.prettierignoreCHANGELOG.mdCONTRIBUTING.mdREADME.mddocs/deployment.mddocs/getting-started.mddocs/troubleshooting.mdeslint.config.jspackage.jsonpackages/cli/bin/llxprtpackages/cli/bin/llxprt.cjspackages/cli/package.jsonpackages/cli/scripts/install-native-launchers.cjspackages/cli/src/launcher/cli-bin.e2e.test.tspackages/cli/src/launcher/cli-bin.test.tspackages/test-utils/src/cli-args.test.tspackages/test-utils/src/cli-args.tspackages/test-utils/src/test-rig.tsscripts/bun-build.config.mjsscripts/postinstall.cjsscripts/start.tsscripts/test-acp-integration.mjsscripts/tests/bun-script-migration.test.tsscripts/tests/issue-2603-install-layouts.test.tsscripts/tests/issue-2603-install-native-launchers-contract.test.tsscripts/tests/issue-2603-install.test.tsscripts/tests/issue-2603-launcher.test.tsscripts/tests/issue-2603-release-install-smoke.cjsscripts/tests/issue-2603-release-install.test.tsscripts/tests/issue-2603-release-pack.cjsscripts/tests/issue-2603-startup-benchmark.cjsscripts/tests/issue-2603-windows-probe.tsscripts/tests/publish-integrity.test.tsscripts/tests/smoke-cli-entry.test.tsscripts/tests/tmux-harness.test.jsscripts/windows-installed-command-smoke.cjsscripts/windows-installed-command-smoke/assert.cjsscripts/windows-installed-command-smoke/checks.cjsscripts/windows-installed-command-smoke/constants.cjsscripts/windows-installed-command-smoke/install-helpers.cjsscripts/windows-installed-command-smoke/launcher-invocation.cjsscripts/windows-installed-command-smoke/package-layout.cjsscripts/windows-installed-command-smoke/process-helpers.cjs
💤 Files with no reviewable changes (3)
- packages/cli/src/launcher/cli-bin.e2e.test.ts
- packages/cli/src/launcher/cli-bin.test.ts
- packages/cli/bin/llxprt.cjs
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/troubleshooting.md`:
- Around line 225-232: Update the Exit Code 43 row in the troubleshooting table
to remove “wrong architecture” from its description, leaving only bundled Bun
runtime conditions that this launcher path actually detects, such as missing,
corrupt, or unusable runtimes.
In `@scripts/lib/npm-command.cjs`:
- Around line 83-115: Update resolveNpmCliJs so the npm_execpath branch only
accepts paths whose basename is npm-cli.js, while retaining the existing .js and
existence checks. Continue using the Node-relative fallback for other paths,
including pnpm or Yarn CLI locations.
In `@scripts/tests/npm-command.test.ts`:
- Around line 108-118: Strengthen the npmInvocation test by supplying arguments
containing spaces, Unicode, and Windows shell metacharacters instead of only
“pack”. Assert the complete inv.args array exactly matches the expected argument
boundaries and values, while retaining the existing command-trimming assertion;
update the test named “never produces a shell string with spaces or
metacharacters”.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2dff43dc-93af-4f3c-a07d-7b62aaafdc76
📒 Files selected for processing (30)
CONTRIBUTING.mdREADME.mddocs/getting-started.mddocs/troubleshooting.mdpackages/cli/bin/llxprtpackages/cli/scripts/install-native-launchers.cjspackages/test-utils/src/cli-args.test.tsscripts/lib/npm-command.cjsscripts/postinstall.cjsscripts/tests/issue-2603-install-layouts.test.tsscripts/tests/issue-2603-install-native-launchers-contract.test.tsscripts/tests/issue-2603-install.test.tsscripts/tests/issue-2603-launcher-signals.test.tsscripts/tests/issue-2603-launcher-source-workspace.test.tsscripts/tests/issue-2603-launcher.test.tsscripts/tests/issue-2603-release-install-smoke.cjsscripts/tests/issue-2603-release-install.test.tsscripts/tests/issue-2603-release-pack.cjsscripts/tests/issue-2603-shim-separator-tests.test.tsscripts/tests/issue-2603-smoke-helpers.test.tsscripts/tests/issue-2603-startup-benchmark.cjsscripts/tests/issue-2603-windows-probe.tsscripts/tests/npm-command.test.tsscripts/tests/postinstall-manager-aware.test.tsscripts/windows-installed-command-smoke.cjsscripts/windows-installed-command-smoke/assert.cjsscripts/windows-installed-command-smoke/checks.cjsscripts/windows-installed-command-smoke/install-helpers.cjsscripts/windows-installed-command-smoke/launcher-invocation.cjsscripts/windows-installed-command-smoke/process-helpers.cjs
🚧 Files skipped from review as they are similar to previous changes (13)
- scripts/windows-installed-command-smoke/assert.cjs
- scripts/postinstall.cjs
- CONTRIBUTING.md
- scripts/windows-installed-command-smoke.cjs
- scripts/windows-installed-command-smoke/launcher-invocation.cjs
- scripts/tests/issue-2603-install-native-launchers-contract.test.ts
- scripts/tests/issue-2603-install-layouts.test.ts
- scripts/tests/issue-2603-windows-probe.ts
- scripts/tests/issue-2603-install.test.ts
- scripts/windows-installed-command-smoke/install-helpers.cjs
- packages/cli/scripts/install-native-launchers.cjs
- scripts/windows-installed-command-smoke/checks.cjs
- scripts/tests/issue-2603-release-pack.cjs
TLDR
Replace the installed Node-to-Bun relay with direct package-local Bun launchers. POSIX installations now use a checked-in shell launcher, while Windows npm installations receive owned native CMD and PowerShell launchers during postinstall. This preserves the no-compile TypeScript runtime and one-command npm installation while removing Node from the normal installed command path.
Important review areas are npm cmd-shim ownership validation, package-relative Bun resolution, Windows argument/exit behavior, and the release-like install fixtures.
Dive Deeper
Reviewer Test Plan
Testing Matrix
Local macOS verification completed for the full test, lint, typecheck, format, build, lockfile, release-install, and live smoke gates. Hosted Windows and Linux validation is provided by PR workflows.
Linked issues / bugs
Fixes #2603
Summary by CodeRabbit
llxprtnow uses a native POSIX launcher (no Node relay) and runs the bundled Bun directly..cmdand.ps1launchers with consistent argument passing and exit-code behavior.